fix(template-no-invalid-aria-attributes): absorb allowundefined handling into validateByType#2723
Draft
johanrd wants to merge 2 commits intoember-cli:masterfrom
Draft
fix(template-no-invalid-aria-attributes): absorb allowundefined handling into validateByType#2723johanrd wants to merge 2 commits intoember-cli:masterfrom
johanrd wants to merge 2 commits intoember-cli:masterfrom
Conversation
3 tasks
…ing into validateByType Removes the top-level string-"undefined" short-circuit in `isValidAriaValue` (previously a two-layer dance: a token-type precheck then a boolean-ish short-circuit). Absorbs the `allowundefined` handling directly into `validateByType` via a new `allowsUndefinedLiteral(attrDef, value)` helper that honors aria-query's convention for the 4 boolean-type attrs that encode "string 'undefined' is spec-valid": aria-expanded, aria-hidden, aria-grabbed, aria-selected. ## Before/after — same outcome, cleaner structure | Attribute | aria-query type / allowundefined | `"undefined"` string accepted? | |---|---|---| | aria-orientation | token / (unset) with `'undefined'` in values | yes — via token-values check | | aria-expanded | boolean / true | yes — via allowundefined | | aria-hidden | boolean / true | yes — via allowundefined | | aria-grabbed | boolean / true | yes — via allowundefined | | aria-selected | boolean / true | yes — via allowundefined | | aria-pressed | tristate / (unset) | NO — no allowundefined flag | | aria-checked | tristate / (unset) | NO — no allowundefined flag | All behavior preserved; just structured so that validity decisions happen in one place. ## Why keep the allowundefined path at all The 4 boolean-type attrs with `allowundefined: true` have spec-valid `"undefined"` values per WAI-ARIA 1.2 (e.g. aria-expanded accepts true/false/undefined). aria-query encodes this. Peers (jsx-a11y, lit-a11y, angular-eslint) effectively reject `aria-expanded="undefined"` because their `allowundefined` branch only fires on JS undefined, not the string. That's a peer bug; our path keeps us spec-compliant. Tests added for each of the 4 allowundefined attrs; new negative test for `aria-pressed="undefined"` (tristate without allowundefined) pins the correct rejection path.
c0e0fcd to
bcadc82
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR is part of a broader a11y parity audit comparing our rules against jsx-a11y, vue-a11y, angular-eslint-template, and lit-a11y.
Premise
Per
aria-query5.3.2, a handful of ARIA attributes accept the literal string"undefined"as a spec-valid value:type/allowundefinedaria-orientationtoken/(unset)with'undefined'in values listaria-expandedboolean/truearia-hiddenboolean/truearia-grabbedboolean/truearia-selectedboolean/trueTwo orthogonal encodings: token with
'undefined'in values (aria-orientation) and boolean withallowundefined: true(the other four). Both should produce "accept the string'undefined'"; neither is the user's fault to write.Problem
The pre-fix
isValidAriaValuehad a clunky two-layer structure:The precheck was a late patch that landed because
aria-orientation="undefined"(the token case) was falsely rejected by the short-circuit. The short-circuit itself exists for the 4 boolean-type attrs. So validation was split across three locations for one attribute.Fix
Absorb
allowundefinedhandling directly intovalidateByTypevia a single helperallowsUndefinedLiteral(attrDef, value). The token-values check in the'token'case naturally handlesaria-orientation="undefined"without any special-casing.isValidAriaValuebecomes a thin dispatcher tovalidateByType.Behavior — unchanged outcomes, cleaner structure
aria-orientation="undefined"aria-expanded="undefined"aria-hidden="undefined"aria-grabbed="undefined"aria-selected="undefined"aria-pressed="undefined"aria-orientation="sideways"Prior art — peers don't handle the 4 boolean+allowundefined attrs correctly
Verified each peer in source:
aria-expanded="undefined"aria-proptypesvalidityCheck('boolean', value)istypeof value === 'boolean'; string"undefined"fails.allowUndefined && value === undefinedonly fires on JSundefined, not the string — and is effectively dead code because jsx-a11y readsattributes.allowUndefined(camelCase) but aria-query emitsallowundefined(lowercase). Rejects the string.aria-attr-valid-valueallowundefinedlowercase matches aria-query correctly, but still only triggers on JSundefined. Rejects the string.valid-ariaif (allowundefined && isNil(attributeValue)) return true;—allowundefinedonly triggers on null/undefined, not the string. Rejects the string.aria-propsaria.has(name). Trivially accepts the string (by not checking).So of the four peers, three reject a spec-valid construct, one doesn't check values at all. This PR makes our rule more spec-compliant than the three value-validating peers on these 4 attrs.
Note — case-insensitivity gap (out of scope)
jsx-a11y and lit-a11y lowercase the value before the token check (
permittedValues.indexOf(value.toLowerCase()) > -1). Our rule does NOT lowercase; a case-variant likearia-orientation="UNDEFINED"or"Horizontal"would still fail here even after this PR. Same class of HTML-enumerated-attribute case-insensitivity gap as #2718 (kind="captions") and #2728 (role tokens). Worth a follow-up PR; explicitly out of scope for this narrow change.Tests
aria-orientation="undefined"/"horizontal",aria-expanded="undefined",aria-hidden="undefined",aria-grabbed="undefined",aria-selected="undefined",aria-pressed="true"/"false"/"mixed".aria-orientation="sideways",aria-pressed="undefined"(no allowundefined),aria-required="undefined"(boolean without allowundefined — pre-existing test).